Skip to content

test(cli): pin JSON array literal handling for array-typed arguments (#2150) - #2160

Open
htarnacki wants to merge 1 commit into
DeusData:mainfrom
htarnacki:fix/cli-json-array-args
Open

test(cli): pin JSON array literal handling for array-typed arguments (#2150)#2160
htarnacki wants to merge 1 commit into
DeusData:mainfrom
htarnacki:fix/cli-json-array-args

Conversation

@htarnacki

@htarnacki htarnacki commented Sep 10, 2026

Copy link
Copy Markdown

What does this PR do?

Fixes #2150.

codebase-memory-mcp cli <tool> --flag value stored a JSON array literal passed to an array-typed flag (--semantic-query '["a","b"]') as a single string element, so the search ran against the literal text. cli_add_typed now expands a value that starts with [ and parses as a non-empty JSON array of strings into individual elements; every other value (repeated flags, plain strings, non-array JSON) keeps the current behaviour. One commit, unit test in test_cli.c.

Checklist

  • Every commit is signed off (git commit -s) — required, CI rejects
    unsigned commits (DCO, see CONTRIBUTING.md)
  • Tests pass locally (make -f Makefile.cbm test) — all suites green except 10 test_cli.c install/uninstall cases that fail identically on a clean main in my environment (activation transaction I/O failed: target-entry-snapshot, environment-specific).
  • Lint passes (make -f Makefile.cbm lint-ci) — clang-format-20 and check-dco.sh clean; clang-tidy / cppcheck are not installed locally, relying on CI for those two.
  • New behavior is covered by a test (reproduce-first for bug fixes)

@htarnacki
htarnacki requested a review from DeusData as a code owner September 10, 2026 08:41
@github-actions

github-actions Bot commented Sep 10, 2026

Copy link
Copy Markdown

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

  • It will not be closed for inactivity. No stale bot touches pull requests here.
  • It may still sit a while before a human reads it. That is on us, not on you.
  • Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

  • Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
  • Get CI green, or say which failures you believe are pre-existing.
  • Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
  • Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

@htarnacki
htarnacki force-pushed the fix/cli-json-array-args branch 2 times, most recently from b3af448 to c1dd441 Compare September 14, 2026 08:38
@DeusData DeusData added bug Something isn't working ux/behavior Display bugs, docs, adoption UX priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. labels Sep 19, 2026
@DeusData

DeusData commented Sep 19, 2026

Copy link
Copy Markdown
Owner

Thank you for the CLI target-array correction. Current argument handling can pass array-looking text as a literal value, matching the reported contract gap. Review should preserve repeatable-flag compatibility and explicitly cover empty, malformed, mixed and repeated inputs. Maintainer bundle #2227 overlaps this area, so we need to reconcile the contributions before integration rather than ask you to redo the same work.

@htarnacki
htarnacki force-pushed the fix/cli-json-array-args branch from c1dd441 to 2e4b459 Compare September 21, 2026 08:00
@htarnacki

htarnacki commented Sep 21, 2026

Copy link
Copy Markdown
Author

Added the explicit coverage you asked for (same base, single commit amended):

input result test
--semantic-query '["a","b"]' ["a","b"] cli_build_args_json_array_literal
--semantic-query '[]' [] (no items; previously became the literal keyword "[]") ..._empty
--semantic-query '["a",' (malformed) unchanged behaviour: one literal string, no error ..._malformed
--semantic-query '[1,2]' (non-string items) unchanged behaviour: one literal string ..._non_string_items
a, '["b","c"]', d (mixed) ["a","b","c","d"] in argument order ..._mixed_with_repeated
'["a"]', '["b"]' (repeated literals) ["a","b"] ..._repeated

The only behavioural change relative to the previous revision is the empty-array case: a well-formed empty array is now expanded (to nothing) instead of falling through to the literal-string path. Everything that is not a well-formed array of strings keeps the pre-existing literal-string behaviour, and the existing repeated-flag test (cli_build_args_json_repeated_array_issue680) is untouched.

@DeusData

DeusData commented Sep 25, 2026

Copy link
Copy Markdown
Owner

Thank you for the table — six explicit cases, with a test named for each, is exactly the coverage we asked for, and the note that only the empty-array case changes behaviour made it easy to review.

Here is the reconciliation I promised with #2227. That PR landed on main on 21 September with the same fix in the same place: cli_add_typed expands a value that parses as a JSON array. That is why your branch now conflicts in src/cli/cli.c. Your implementation and ours differ in only one case:

input main (#2227) this PR
'["a","b"]', '[]', mixed, repeated expanded, in argument order same
'["a",' (malformed) one literal string same
'[1,2]' (non-string items) items added as typed values (1, 2) one literal string "[1,2]"

What main does not have is your tests, and they are the part we would most like to keep. Could you rebase onto current main, keep main's cli_add_typed block (drop your copy of it), and keep your test cases? Your ..._non_string_items test would then assert main's typed-copy behaviour instead of the literal string. If you think the literal-string behaviour is the better contract for a string-typed flag like --semantic-query, say so and we'll discuss it. It's a fair point, but it belongs in its own small change.

Thank you again, and sorry that two PRs ended up fixing the same gap at the same time.

@htarnacki
test(cli): pin JSON array literal handling for array-typed arguments
bc960d8 
`codebase-memory-mcp cli <tool> --flag value` builds the tool's JSON
arguments from typed flags. Array-typed parameters such as
`semantic_query` used to accept only repeated flags; the JSON spelling
shown in the MCP schema (`--semantic-query '["a","b"]'`) was stored as a
single opaque string element (DeusData#2150). `cli_add_typed` now expands such a
literal; this pins the edge cases of that contract so it cannot regress:

- a well-formed array of strings contributes its elements
- an explicit empty array `[]` contributes nothing (not the keyword "[]")
- a malformed literal stays one literal string, without an error
- non-string items are copied as typed values, not stringified
- literals and repeated flags mix, accumulating in argument order
- repeated literals accumulate

Signed-off-by: Hubert Tarnacki <hubert.tarnacki@gmail.com>
@htarnacki

htarnacki commented Sep 26, 2026

Copy link
Copy Markdown
Author

Rebased onto current main (64c23fab) as asked:

Local run: all 15 cli_build_args_json_* tests pass (the six from this PR plus main's nine).

On the literal-string vs. typed-copy question: I don't feel strongly enough to push for a separate change. For a string-typed array like --semantic-query, the server-side schema validation is the right place to reject [1,2] and the CLI copying the values through as-is keeps it a transparent serializer, which is the simpler contract. Happy to leave it as main has it.

@htarnacki
htarnacki force-pushed the fix/cli-json-array-args branch from 2e4b459 to bc960d8 Compare September 26, 2026 07:43
@htarnacki htarnacki changed the title fix(cli): accept JSON array literals for array-typed arguments (#2150) test(cli): pin JSON array literal handling for array-typed arguments (#2150) Sep 26, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DeusData DeusData Awaiting requested review from DeusData DeusData is a code owner

Assignees

No one assigned

Labels

bug Something isn't working priority/high Needs near-term maintainer attention; high-impact bug, regression, safety issue, or release blocker. ux/behavior Display bugs, docs, adoption UX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

cli: JSON array value for array-typed flags (--semantic-query '["a","b"]') is stored as a single string element

2 participants

@htarnacki @DeusData